Skip to content

chore: fix new unnecessary gosec warnings - #1461

Merged
openshift-merge-bot[bot] merged 1 commit into
redhat-developer:masterfrom
jgwest:fix-gosec-oct-2026
Oct 24, 2026
Merged

chore: fix new unnecessary gosec warnings#1461
openshift-merge-bot[bot] merged 1 commit into
redhat-developer:masterfrom
jgwest:fix-gosec-oct-2026

Conversation

@jgwest

@jgwest jgwest commented Oct 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR do?

  • A new version of gosec is reporting new warnings, causing CI failures
  • This PR adds the appropriate tags to handle these warnings

PR acceptance criteria:

  • Documentation was updated and verified using make serve-docs
  • Unit tests were updated
  • E2E tests were updated

What type of PR is this?
/kind failing-ci

@jgwest
chore: fix new unnecessary gosec warnings
8b39d45 
Signed-off-by: Jonathan West <jgwest@gmail.com>
@openshift-ci openshift-ci Bot added the kind/failing-ci OpenShift CI failure label Oct 23, 2026
@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between b61fc0a and 8b39d45.

📒 Files selected for processing: 7
  • argocd-operator/controllers/argocd/dex.go
  • argocd-operator/controllers/argocd/image_updater.go
  • argocd-operator/controllers/argocdagent/agent/deployment.go
  • argocd-operator/controllers/argocdagent/deployment.go
  • argocd-operator/tests/auxiliary/smtplistener/cmd/listener/serve.go
  • argocd-operator/tests/ginkgo/fixture/argocd/fixture.go
  • argocd-operator/tests/ginkgo/fixture/gitserver/repo.go
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Failures during configuration reloads triggered by SIGHUP and log reopening triggered by SIGUSR1 are now reported in logs rather than silently ignored. This makes signal-related operational problems easier to notice and diagnose. The underlying operations and handling of other signals remain unchanged; only failure reporting has changed.

The diff adds gosec annotations to credential-like literals and test fixture code. It also logs errors returned by configuration reload and log reopening in the SMTP listener’s SIGHUP and SIGUSR1 handlers.

Changes: Gosec annotations

Layer / File(s) Summary
Operator configuration and deployment annotations
argocd-operator/controllers/argocd/dex.go, argocd-operator/controllers/argocd/image_updater.go, argocd-operator/controllers/argocdagent/agent/deployment.go, argocd-operator/controllers/argocdagent/deployment.go		 Added #nosec G101 annotations to the Dex clientSecret placeholder and Secret resource names. The configuration values and volume behavior remain unchanged.
Test fixture annotations
argocd-operator/tests/ginkgo/fixture/argocd/fixture.go, argocd-operator/tests/ginkgo/fixture/gitserver/repo.go		 Added #nosec G124 and #nosec G204 comments for the outbound cookie and Git command construction. Executable behavior remains unchanged.

SMTP listener signal error logging

Layer / File(s) Summary
Log signal operation errors
argocd-operator/tests/auxiliary/smtplistener/cmd/listener/serve.go		 The SIGHUP and SIGUSR1 handlers now log errors returned by configuration reload and log reopening. Previously, both errors were ignored.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Suggested reviewers: olivergondza, jaideepr97

Merge Risk: ⚪ Minimal · up to 8b39d

The PR is mergeable with no identified user-facing or operational risk.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 7 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the gosec warning fixes, which match the main changes.
Description check Passed The description accurately states that the PR addresses new gosec warnings by adding tags.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@openshift-ci
openshift-ci Bot requested review from Rizwana777 and jparsai October 23, 2026 12:08
@chengfang

chengfang commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

/lgtm
/approve

@openshift-ci openshift-ci Bot added the lgtm label Oct 23, 2026
@openshift-ci

openshift-ci Bot commented Oct 23, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chengfang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@chengfang chengfang mentioned this pull request Oct 23, 2026
Open
3 tasks
ranakan19
ranakan19 approved these changes Oct 23, 2026
View reviewed changes

@ranakan19 ranakan19 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
/lgtm

Comment thread argocd-operator/tests/ginkgo/fixture/argocd/fixture.go

req = req.WithContext(ctx)

// #nosec G124 -- this is an outbound request cookie (client to server); Secure/HttpOnly/SameSite are response-only attributes and don't apply here

@ranakan19 ranakan19 Oct 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ranakan19

ranakan19 commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

/retest
terraform error; failed to create cluster

@chengfang

chengfang commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

/test v4.14-kuttl-parallel

@olivergondza

olivergondza commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator

/test v4.14-ci-index-gitops-operator-bundle

@olivergondza

olivergondza commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator

/test v4.14-images

@olivergondza

olivergondza commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator

/retest-required

@openshift-merge-bot
openshift-merge-bot Bot merged commit 1b4dd31 into redhat-developer:master Oct 24, 2026
19 checks passed
This was referenced Oct 24, 2026
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jparsai jparsai Awaiting requested review from jparsai
@Rizwana777 Rizwana777 Awaiting requested review from Rizwana777
1 more reviewer
@ranakan19 ranakan19 ranakan19 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@chengfang chengfang

@ranakan19 ranakan19

Labels

approved kind/failing-ci OpenShift CI failure lgtm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jgwest @chengfang @ranakan19 @olivergondza